fix(rtp): disable header extension aggregation on every depayloader by wagenet · Pull Request #721 · Eyevinn/strom

wagenet · 2026-08-27T23:16:06Z

Problem

A single corrupt RTP stream can abort the whole process, killing every unrelated flow on the server:

gstrtpbasedepayload.c:942:gst_rtp_base_depayload_handle_buffer:
'gst_buffer_list_length (priv->hdrext_buffers) == 0' should be TRUE

Since 1.24 GstRTPBaseDepayload caches each packet's RTP header while assembling an output buffer, clearing it only when the subclass pushes or flushes. rtph264depay breaks that invariant: an interrupted fragmentation unit calls delayed() then finish_fragmentation_unit(), which in access-unit mode can absorb the truncated NAL without pushing anything. The cache stays populated and the assert fires. g_assert_true is not defusable.

Upstream: gstreamer#5057, unfixed.

Fix

Disable header-extension aggregation on every depayloader. That restores pre-1.24 behaviour — extensions are read from the current packet instead of accumulated — so the cache never fills.

Nothing is lost: Strom reads no header-extension metadata, and the extensions that matter for transport (transport-cc, abs-send-time, mid) are consumed by webrtcbin well upstream of any depayloader.

install() sweeps depayloaders already present in a pipeline and connects deep-element-added for ones autoplugged later. The closure captures nothing, so it creates no reference cycle.

Because the abort takes down the process rather than one flow, it does not matter which pipeline hosts the offending depayloader. Strom runs three kinds that can host one, and all three now call install() while still in NULL:

Pipeline	How a depayloader gets there
Main (`PipelineManager::start()`)	`decodebin` autoplugs one, or a user places one directly in a flow
Media Player internal (decode + passthrough)	`uridecodebin` / `urisourcebin` autoplug one from an `rtsp://` URI
WHIP Input session	`whipserversrc` autoplugs them inside its own bin

Why resolve the symbol at runtime

There is no GObject property — the C setter is the only switch, and it is Since: 1.24. Binding it normally would raise the workspace build floor from 1.22, breaking Debian 12 and the ARM64 cross-compile target (Raspberry Pi OS 12), which both ship 1.22. Resolving at runtime keeps one binary working on both: below 1.24 the lookup fails and we do nothing, which is correct, because the bug doesn't exist there.

dlsym(RTLD_DEFAULT, …) on Unix; on Windows GetModuleHandleEx(FROM_ADDRESS) anchored on an already-linked RTP symbol, to avoid hardcoding a DLL name that varies between builds.

Tests

Six unit tests plus one integration test, all verified to fail when the fix is reverted:

Revert	What fails
`install()` neutered	the two `install_disables_aggregation_*` unit tests
`install()` call removed from `start()`	`rtp_hdrext_aggregation_test`
`install()` calls removed from the Media Player bridge	`decode_pipeline_disables_hdrext_aggregation`, `passthrough_pipeline_disables_hdrext_aggregation`

The rtp_hdrext unit tests can't see their call sites, hence the separate tests per site. The two Media Player tests call the real create_decode_pipeline / create_passthrough_pipeline, then add a depayloader to a nested bin — the same path deep-element-added sees for an autoplugged one — because CI cannot serve an RTSP stream.

rtph264depay_enables_aggregation_by_default confirms the unpatched default really is on, so the getter reads live state rather than always returning false. symbol_resolves_on_modern_gstreamer fails loudly if the lookup ever breaks on >= 1.24 — otherwise the workaround would silently no-op.

rtph264depay is in gstreamer1.0-plugins-good, already in CI, so these run rather than skip.

Gap: the WHIP Input call site has no test. build_whipserversrc binds a TCP port and needs a full endpoint config, so there is no cheap way to construct one in a unit test. It is covered by inspection only.

What I ran

On macOS, GStreamer >= 1.24: full cargo test (543 lib + all integration, 0 failures), cargo clippy --all-targets -D warnings (zero diagnostics), cargo fmt --check, pipeline_lifecycle_test 3/3 including the circular-reference detector, and every revert scenario in the table above.

The macOS and Windows builds are off for push/PR, so this PR's own check list skips them and the platform-specific FFI would otherwise merge never having compiled. Dispatched manually on the fork, where the branch lives, since workflow_dispatch on this repo needs admin: run 33179848797 on 55e91ef — all green, including Build (macOS) and Build (Windows). Both ran Clippy and the test suite on their own OS, so the Windows GetModuleHandleEx/GetProcAddress path has now compiled and linked for the first time, and the tests above executed on macOS and Windows as well as Linux.

Still not exercised anywhere: the < 1.24 no-op path — no CI image ships 1.22, and every test early-returns when is_supported() is false. It is the trivial branch (lookup fails, install() returns), but it is untested.

Since 1.24 GstRTPBaseDepayload caches the RTP header of every packet feeding the output buffer it is assembling, clearing the cache only when the subclass pushes or flushes. gst_rtp_base_depayload_delayed() means "this packet's header belongs to the next output buffer", and the base class asserts the cache is empty when that happens. rtph264depay breaks that invariant: an interrupted fragmentation unit calls delayed() and then finish_fragmentation_unit(), which in access-unit mode can absorb the truncated NAL without producing an output buffer. Nothing is pushed, the cache is still populated, and gstrtpbasedepayload.c:942:gst_rtp_base_depayload_handle_buffer: 'gst_buffer_list_length (priv->hdrext_buffers) == 0' should be TRUE aborts the process, taking every unrelated flow on the server with it. g_assert_true is not defusable, so the only fix available to us is to stop the cache from filling. Turning aggregation off restores the pre-1.24 behaviour: header extensions are read from the current packet instead of accumulated. Strom reads no header-extension metadata, and the extensions that matter for transport (transport-cc, abs-send-time, mid) are consumed by webrtcbin well upstream of any depayloader, so nothing is lost. There is no GObject property for this - the C setter is the only switch, and it is Since: 1.24. Binding it normally would raise the workspace build floor from 1.22, breaking the default install on Debian 12 and the default ARM64 cross-compile target (Raspberry Pi OS 12), both of which ship GStreamer 1.22. Resolving the symbol at runtime keeps one binary working on both: below 1.24 the lookup fails and we do nothing, which is correct because aggregation - and therefore the bug - does not exist there. install() sweeps elements already in the pipeline and connects deep-element-added for ones decodebin autoplugs later. It runs at the top of start(), before any state change, so no depayloader is missed. The handler captures nothing, so it creates no reference cycle. Refs: https://gitlab.freedesktop.org/gstreamer/gstreamer/-/issues/5057 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E2xUTyh7HkjawtC8CxMwh6

srperens

Verdict: Comment — sound mechanism and test discipline for the pipeline it touches, but the fix is narrower than its own framing ("disable ... on every depayloader"), and its platform-specific code is unverified by CI.

Requested changes

Also call rtp_hdrext::install() on the Media Player block's internal pipelines — backend/src/blocks/builtin/mediaplayer/bridge.rs:28 and :138 each build a separate gst::Pipeline running uridecodebin/urisourcebin, which will autoplug an RTP depayloader for an rtsp:// source URI exactly like decodebin does in the main pipeline. That pipeline is never passed to rtp_hdrext::install(), so an interrupted RTSP H264 stream through a Media Player block still takes the whole process down with the bug this PR exists to fix.

Claims

Claim	Verdict	Evidence
`install()` runs before any pipeline state change	CONFIRMED	`backend/src/gst/pipeline/lifecycle.rs:17` `rtp_hdrext::install(&self.pipeline);` runs before `set_state(Ready)` at `lifecycle.rs:65-67` and `set_state(Playing)` later
`deep-element-added` closure captures nothing, no ref cycle	CONFIRMED	`backend/src/gst/rtp_hdrext.rs:177` `bin.connect("deep-element-added", false, move \|args\| {` reads only `args`, no external variable moved in
Covers "every depayloader" the process runs	BOUNDED/CONTRADICTED as stated	Only reaches `self.pipeline`'s bin tree (`rtp_hdrext.rs:168` `iterate_recurse()` + the signal above). Verified two other independent `gst::Pipeline` instances exist and are never passed to `install()`: WHIP Input's `session_pipeline` (`backend/src/blocks/builtin/whip.rs:570`) and Media Player's internal pipelines (`bridge.rs:28`, `:138`). The WHIP one is fine — issue #685 itself traced the crash to the main-pipeline `decodebin`, not an element inside `whipserversrc` — but Media Player's `uridecodebin`/`urisourcebin` is a real, unguarded second path
`libgstrtp-1.0` is already loaded so `dlsym(RTLD_DEFAULT, ...)` finds the symbol	EXTERNAL	Dynamic-linker behavior, not settleable from this repo; the new `gstreamer-rtp` workspace dependency (`Cargo.toml:33`, `backend/Cargo.toml:58`) does cause the lib to link at load time on ELF/Mach-O, which supports the assumption
Windows `GetModuleHandleExA`/`GetProcAddress` path and the Unix `dlsym` path both work as written	UNVERIFIED	`Build (macOS)` and `Build (Windows)` both show `skipping` on this PR's checks (head `c7db33a8`); per CLAUDE.md/CI, neither builds on push/PR, so this is the only new platform-specific code in the diff and it has never compiled

Diagnosis — Root cause and mechanism match issue #685's own confirmed investigation (interrupted FU-A with contiguous sequence numbers on a depayloader that has the X-bit-triggered aggregation cache populated); the "why runtime dlsym, not a feature bump" tradeoff is well-argued and consistent with the workspace's v1_22 floor (Cargo.toml:29). The gap isn't the mechanism, it's coverage: BOUNDED to pipelines the code actually calls install() on, and today that's one of at least three that host GStreamer elements capable of autoplugging an RTP depayloader.

Tests & CI — Linux x86_64/ARM64, Check, WASM, API Contract all pass on c7db33a8. The new unit tests and rtp_hdrext_aggregation_test.rs genuinely exercise PipelineManager::start() (not a reimplementation) and read as revert-sensitive per the PR's own table. macOS/Windows: skipped, so the platform-specific FFI is unverified — dispatch with gh workflow run ci.yml --ref wagenet/685-hdrext-aggregation -f platforms=macos (and =windows) before merge.

Confidence: HIGH

…he main one The abort in GstRTPBaseDepayload (gstreamer#5057) kills the process, so it does not matter which pipeline hosts the depayloader that trips it. Three pipelines can host one, and only the main pipeline was covered: - Media Player's internal pipelines autoplug a depayloader from an rtsp:// URI through uridecodebin/urisourcebin. - whipserversrc autoplugs depayloaders inside its own bin. install() is a no-op below GStreamer 1.24 and only clears a boolean, and all three call sites run while the pipeline is still NULL, so no depayloader is missed. The two new tests call the real Media Player constructors and then add a depayloader to a nested bin — the path deep-element-added sees, since CI cannot serve RTSP. Both fail with the install() calls removed (verified). There is no equivalent test for the WHIP path: build_whipserversrc binds a port and needs a full endpoint config. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KcQpknvxTmHSLzsJPaeq61

wagenet · 2026-08-28T15:14:10Z

Pushed 55e91ef and rewrote the PR description. Both points from the review are addressed.

Coverage. The review was right that "every depayloader" overstated the diff — install() only ever ran on the main pipeline. It now also runs on the Media Player block's two internal pipelines (bridge.rs, decode and passthrough), where an rtsp:// URI makes uridecodebin/urisourcebin autoplug a depayloader.

I went one further and covered WHIP Input's session_pipeline too, which the review judged out of scope. The reasoning there was that #685 traced the crash to the main-pipeline decodebin, not to anything inside whipserversrc — true, but whipserversrc does autoplug depayloaders in its own bin, and g_assert_true aborts the process rather than the flow. Which pipeline hosts the offending depayloader doesn't change the outcome, so leaving it out would keep the title overstating the fix. It is one line, and install() is a no-op below 1.24.

Platform verification. Build (macOS) and Build (Windows) skip on push/PR by design (if: github.event_name == 'workflow_dispatch'), so the new FFI would have merged without ever compiling. workflow_dispatch on this repo needs admin, so I dispatched it on the fork where the branch lives: run 33179848797 on 55e91ef, all green. Both jobs ran Clippy and the test suite on their own OS, so the Windows GetModuleHandleEx/GetProcAddress path has now compiled and linked, and the rtp_hdrext tests executed on macOS and Windows as well as Linux. That run lives on the fork and won't appear in this PR's check list.

Tests. Two new unit tests call the real Media Player constructors, then add a depayloader to a nested bin — the same path deep-element-added sees for an autoplugged one, since CI can't serve RTSP. Both fail with the install() calls removed; I verified that by commenting them out rather than assuming it. Locally: 543 lib tests, all integration tests, clippy --all-targets -D warnings, fmt --check, all clean.

Two gaps left, both now stated in the description: the WHIP call site has no test (build_whipserversrc binds a port and needs a full endpoint config), and the < 1.24 no-op path is untested anywhere, since no CI image ships 1.22 and every test early-returns when is_supported() is false.

_{Written by Claude Code on behalf of @wagenet.}

srperens

Supersedes my 2026-08-28 v3 review (id 5049165959, COMMENTED) at head c7db33a8. That review requested one change: also call rtp_hdrext::install() on the Media Player's internal pipelines. This head does that, plus adds it to WHIP Input for completeness. Verdict changes to Comment→Comment (same), gap closed.

Verdict: Comment — the requested-change gap is closed with genuine coverage; only the pre-existing macOS/Windows CI gap remains, which blocks approval per protocol regardless of the fork run cited in the PR body.

Claims

Claim	Verdict	Evidence
Media Player's two internal pipelines now call `install()` before any state change	CONFIRMED	`backend/src/blocks/builtin/mediaplayer/bridge.rs:129` and `:326`, both immediately before `Ok(pipeline)`; the pipeline is only later moved to `Playing` from `connect_main_pipeline_handler`, invoked asynchronously from `builder.rs:236-247` after construction returns — install always precedes any state transition
WHIP Input's `session_pipeline` now calls `install()` before `Playing`	CONFIRMED	`backend/src/blocks/builtin/whip.rs:1040` `rtp_hdrext::install(&session_pipeline);` sits between the `.add(&whipserversrc)` call and `.set_state(gst::State::Playing)` at line ~1046
New Media Player tests exercise the real constructors, not a reimplementation	CONFIRMED	`bridge.rs:689` and `:700` call `create_decode_pipeline(...)` / `create_passthrough_pipeline(...)` directly, then add a depayloader to a nested `gst::Bin` — the same path `deep-element-added` observes for an autoplugged element — and assert `rtp_hdrext::is_enabled(&depay) == Some(false)`
`gstreamer_rtp::RTPBaseDepayload` and `rtp_hdrext::is_enabled`/`is_supported` used by the new tests exist and match call signatures	CONFIRMED	`backend/Cargo.toml:58` `gstreamer-rtp.workspace = true`; `backend/src/gst/rtp_hdrext.rs:142,150,159` define `is_enabled`, `is_supported`, `install` with matching signatures
macOS/Windows FFI path now verified by this repo's CI	CONTRADICTED as stated	`gh pr checks 721` at head `55e91efa` still shows `Build (Windows)` and `Build (macOS)` as `skipping`, unchanged from the prior review. The green run cited in the PR body (`github.com/wagenet/strom/actions/runs/33179848797`) is on the author's fork, not `Eyevinn/strom` — it is not this repo's CI

Diagnosis — unchanged from my prior review; mechanism and root cause still check out.

Radius — GLOBAL, as before (pipeline lifecycle, three independent gst::Pipeline hosts), now fully covered rather than bounded to one.

Tests & CI — Build (Linux x86_64/ARM64), Check (Linux), Check & Build (WASM), API Contract Check all pass on 55e91efa. Build (macOS)/Build (Windows) still skip on push/PR per repo config — this remains the only new platform-specific code (dlsym/GetModuleHandleEx) never compiled by this repo's own CI. Dispatch before merge: gh workflow run ci.yml --ref wagenet/685-hdrext-aggregation -f platforms=macos (and =windows).

Confidence: HIGH

…message (#751) The onward message came out as prose naming several items by bare number, which is the shape it is now hardest for a reader to act on: a bare #721 autolinks only inside this repository, so everywhere else each reference is four characters someone has to go look up by hand, and a message naming six items costs six searches. The old rule caused it. "Keep it to five lines. Never put a credential, a remote URL or a raw log excerpt in it" banned the very links that make the message useful, leaving bare refs as the only option, and a five-line ceiling with no example pushed items together into sentences. So: every reference is a full URL, every item is its own numbered line, and the ban now names what actually must not appear -- credentials, service endpoints, log excerpts, links outside this repository. The line ceiling becomes twelve item lines, with needs_human kept whole and the rest collapsed into the trailing count, because the onward message is a prompt to act and the full record is in the summary comment. Both link forms are given, since the destination is deployment configuration and this file deliberately does not know which one it is. The /issues/ path resolves pull requests too, so one form covers both and a run never has to determine which an item is. Added a worked example, because README.md is right that a rule describing a shape drifts and an example does not -- the absence of one here is most of why this drifted. Also dropped the `[#721](url)` placeholder from the rendered-half table: that half is posted in this repository, where #721 autolinks on its own, and leaving a fake URL there contradicted the new rule about where each form belongs. Recorded the asymmetry in README.md's design notes so it does not get unified away on the shorter form later. The special case is the point. scripts/agent/test-agent-scripts.sh: 40 passed, 0 failed (unchanged -- this commit touches no script). Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

wagenet · 2026-09-05T15:06:10Z

(from Claude, on behalf of @wagenet)

This PR is unapproved on the Build (macOS) coverage gap alone — no code change is requested. That blocker is shared with #721, #722, #726 and #735, and the documented remedy (gh workflow run ci.yml --ref <branch> -f platforms=macos) appears not to work for fork branches. Written up once in #735 (comment) rather than repeated on each PR.

wagenet marked this pull request as ready for review August 27, 2026 23:37

wagenet mentioned this pull request Aug 28, 2026

fix(whip,whep): disable RTX to avoid a gst-plugins-base crash Eyevinn/open-live#153

Closed

srperens reviewed Aug 28, 2026

View reviewed changes

srperens mentioned this pull request Aug 28, 2026

Agent triage log #679

Open

srperens reviewed Aug 31, 2026

View reviewed changes

This was referenced Sep 4, 2026

fix(whip): stop an unstamped buffer at the session bridge before qtmux sees it #759

Draft

fix(macos): keep the headless server out of App Nap #735

Open

This was referenced Sep 5, 2026

feat(monitor): sample per-thread CPU on macOS via mach thread_info #722

Open

perf(gpu): let macOS choose its own convert mode, and thread the conversion #726

Open

Navigation Menu

Uh oh!

fix(rtp): disable header extension aggregation on every depayloader - #721

fix(rtp): disable header extension aggregation on every depayloader#721
wagenet wants to merge 2 commits into
Eyevinn:mainfrom
wagenet:wagenet/685-hdrext-aggregation

wagenet commented Aug 27, 2026 •

edited

Loading

Uh oh!

srperens left a comment

Uh oh!

wagenet commented Aug 28, 2026

Uh oh!

srperens left a comment

Uh oh!

wagenet commented Sep 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

wagenet commented Aug 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Fix

Why resolve the symbol at runtime

Tests

What I ran

Uh oh!

srperens left a comment

Choose a reason for hiding this comment

Uh oh!

wagenet commented Aug 28, 2026

Uh oh!

srperens left a comment

Choose a reason for hiding this comment

Uh oh!

wagenet commented Sep 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

wagenet commented Aug 27, 2026 •

edited

Loading